Fixes infinite recursion introduced by patch to SplFixedArray #8105
Conversation
|
I was trying to avoid changing the memory layout of the FixedArray if possible in a patch release for a non-security change (e.g. in case FFI or debuggers relied on exact memory layout), but that seems excessive in retrospect.
Either reverting the previous patch or changing the handling of var_export so that class implementations aren't responsible for it seems possibly better, though there's less motivation for the latter approach with nothing other than SplFixedArray in php-src having custom implementations and cyclic data structures and SplFixedArray having limited use. |
ext/spl/spl_fixedarray.c
Outdated
| if (!spl_fixedarray_object_needs_rebuild(intern, ht, j)) { | ||
| /* Return the same hash table so that recursion cycle detection works in internal functions. */ | ||
| return ht; | ||
| } |
There was a problem hiding this comment.
Interesting approach, but I afraid it's too expensive. I thought about setting some flag on SplFixedArray modification.
BTW this leads me to another idea - we may try to return the same ht if it's already protected from recursion.
if (GC_IS_RECURSIVE(ht)) {
return ht;
}Ideally, we should switch from get_properties to get_properties_for and do this for everything except ZEND_PROP_PURPOSE_ARRAY_CAST. Thanks for persistence. I hope, we will fix this soon.
Now, I also see a mesh in ext/standard/var.c that uses different protection mechanisms in different places. Did you propose a PR that tried to change this? I suppose, Z_IS_RECURSIVE_P/GC_IS_RECURSIVE were used on purpose, but I can't remember anything. @nikic do you have any thoughts?
There was a problem hiding this comment.
Now, I also see a mesh in ext/standard/var.c that uses different protection mechanisms in different places. Did you propose a PR that tried to change this?
Yes, I'd proposed #8046 Make var_export/debug_zval_dump first check for infinite recursion on the object to try to change this to always check for infinite recursion on the object before get_properties/get_properties_for. Could you review that?
I suppose,
Z_IS_RECURSIVE_P/GC_IS_RECURSIVEwere used on purpose, but I can't remember anything.
I also don't know, brainstorming ideas of why:
- Trying to get mixes of json_encode calling var_export to work instead of failing with
RECURSION? - seems obscure and not likely. There are alternate approaches even if that really was the reason for the approach, e.g. newGC_FLAGSbits specifically for var_export/debug dumpers - Trying to detect sharing the same hash table - I can't think of anything doing that
There was a problem hiding this comment.
BTW this leads me to another idea - we may try to return the same ht if it's already protected from recursion.
if (GC_IS_RECURSIVE(ht)) { return ht; }
It's protected from recursion when debugging - var_export/var_dump would show the wrong results if the side effects of __debugInfo, jsonSerialize, etc. of another method modified the underlying table during a json_encode/var_dump call, but it wouldn't crash or misbehave.
That's probably work in combination with adding an implementation of get_properties_for for other ZEND_PROP_PURPOSE_*, though with all the problems this has had so far, I'd rather introduce that in 8.2 rather than in a patch release. (especially with SplFixedArray being subclassable and subclasses being able to override JsonSerializable, __sleep, __debugInfo, etc.
(because json_encode, var_export, debug_zval_dump wouldn't use the hash table, they'd just report recursion).
There was a problem hiding this comment.
Interesting approach, but I afraid it's too expensive. I thought about setting some flag on SplFixedArray modification.
I pushed a new commit doing that instead
Closes phpGH-8079 Track whether the spl_fixedarray was modified since the last call to get_properties
3daa8e3 to
6e736d1
Compare
Closes GH-8079
Under most circumstances, the zend_hash_index_update is setting the value to
itself when the hash table's reference count is already 2